-
Notifications
You must be signed in to change notification settings - Fork 786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Config encrypt #1204
Config encrypt #1204
Conversation
…ust encrypt it and send it back to the frontend
…g is exported correctly
…t's about configuration and renamed it to UnsafeConfigOptionsConfirmationModal
…ngifying) and front end (unsafe import warning overlay)
…riables and extracting methods.
DeepCode's analysis on #57f35f found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin. |
Do we need to add documentation about this? I'm not sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a partial review of this. Before we spend more time on this, I'd like to make sure we checked with Barak that this is the best path. I think it's a better user experience just to give them a checkbox that lets them exclude sensitive information from the export. It's fewer clicks, less complex, and we don't have to worry about any compliance issues.
Additionally, this project is already more complex than we our team can really handle. We need to be removing complexity, not adding it. I think a warning to the user that the exported config contains the credentials is all that's really necessary to mitigate any security risk. Adding a checkbox to exclude the credentials is also straight forward and requires no changes to the import process.
Finally, since the scenarios feature will fundamentally change the way that users interact with configuration, I don't think we should be investing too much time adding in complexities to the existing import/export capability.
In summary, I don't think the cost of the complexity is worth it.
monkey/tests/unit_tests/monkey_island/cc/services/utils/cyphertexts_for_encryption_test.py
Outdated
Show resolved
Hide resolved
monkey/monkey_island/cc/ui/src/components/configuration-components/ExportConfigModal.tsx
Outdated
Show resolved
Hide resolved
monkey/tests/unit_tests/monkey_island/cc/services/utils/test_config_encryption.py
Outdated
Show resolved
Hide resolved
monkey/tests/unit_tests/monkey_island/cc/services/utils/test_config_encryption.py
Outdated
Show resolved
Hide resolved
More complex to implement and maintain, as we can't always know what's sensitive and what's not. What compliance issues do we worry about? Just make the encryption an industry standard and if it's not enough user can always encrypt manually.
So a warning means that the user needs a popup or some other UI element to be introduced to the security concern. If we need this, we might as well offer a proper solution to this problem right there and then. Re-entering all of the sensitive fields is definitely not more UX friendly than just entering a password for an export.
I don't see any fundamental changes for configuration on the horizon. Scenarios will use configuration, so scenario export encryption == config export encryption.
Maybe a better solution would've been to just encrypt by default based on user's credentials. The argument that we'd get a lot of support emails is not that valid IMO, for multiple reasons:
|
…word -> test_encrypt_decrypt_config__decrypt_no_password
…one for too short configuration, another one for corrupted file.
Codecov Report
@@ Coverage Diff @@
## develop #1204 +/- ##
===========================================
+ Coverage 28.63% 28.73% +0.09%
===========================================
Files 427 434 +7
Lines 12888 13192 +304
===========================================
+ Hits 3691 3791 +100
- Misses 9197 9401 +204
Continue to review full report at Codecov.
|
Co-authored-by: Shreya Malviya <[email protected]>
…uration is encrypted or not. This solved a bug where invalid json was treated as credential error.
…ks whether or not config is encrypted.
...island/cc/ui/src/components/configuration-components/UnsafeConfigOptionsConfirmationModal.js
Outdated
Show resolved
Hide resolved
monkey/monkey_island/cc/ui/src/components/pages/ConfigurePage.js
Outdated
Show resolved
Hide resolved
1aa5083
to
57f35f9
Compare
What does this PR do?
Fixes #1172
Add any further explanations here.
PR Checklist
Testing Checklist